vendor: github.com/moby/go-archive main / v0.3.0-dev#7111
Conversation
full diff: moby/go-archive@v0.2.0...91b7e54 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
full diff: ctalledo/go-archive@91b7e54...df2ce62 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
This vendor update of github.com/moby/go-archive introduces significant security hardening — os.Root-based filesystem operations with openat(2) semantics, filepath.IsLocal path validation, and improved AUFS whiteout handling. The overall direction is a clear improvement.
Two security findings in the new code warrant attention before merging:
- TOCTOU in opaque-whiteout walk (
diff.go:151):os.RemoveAll(p)uses an absolute path outside theos.Rootcontainment boundary, unlike every other removal in the same function. - Empty tar entry name (
diff.go:62): A tar entry with an empty name normalizes to"."(the extraction root), passesfilepath.IsLocal, and can overwrite root directory metadata.
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
This vendor update introduces -based containment for tar extraction (using semantics), replaces manual path-traversal checks with , hardens device-number overflow detection, and updates the implementation to use a parent-directory file descriptor. The security model is sound and the hardening changes are a clear improvement over the previous string-prefix checks.
b9966d4 to
30091c1
Compare
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
This PR vendors github.com/moby/go-archive from v0.2.0 to a pre-release dev version (v0.2.1-0.20260716170833 via a replace directive pointing to ctalledo's fork). The core changes are security hardening of the tar extraction path: Unpack and createTarFile now use Go's os.Root API (openat-backed bounded filesystem operations) instead of raw os.* calls with manual prefix checks, and path traversal validation is strengthened with filepath.IsLocal after path.Clean.
The hardening changes are well-structured. The os.Root API provides the primary containment guarantee for all path operations. All drafter findings were low-severity and, on deeper analysis, confirmed to be either correct behavior or no-bug outcomes. No high or medium severity issues were identified.
Note: vendor.mod contains a // FIXME comment indicating this uses a personal fork (ctalledo/go-archive) for testing moby/go-archive#45. This is explicitly called out in the PR description and is expected for a dev-version vendor bump.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
30091c1 to
6ab7157
Compare
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🔴 CRITICAL
This PR vendors an updated github.com/moby/go-archive (v0.3.0-dev) with security hardening, but the vendor.mod includes a replace directive that substitutes the canonical module with a personal testing fork (ctalledo/go-archive). This was intentionally marked FIXME and must be removed before merging.
Lower-confidence findings (not posted inline)
None
| ) | ||
|
|
||
| // FIXME(thaJeztah): testing https://github.com/moby/go-archive/pull/45 | ||
| replace github.com/moby/go-archive => github.com/ctalledo/go-archive v0.2.1-0.20260717121834-2bc39549db4e |
There was a problem hiding this comment.
[high] replace directive points to a personal testing fork instead of the canonical moby/go-archive
The PR adds a replace directive in vendor.mod that redirects github.com/moby/go-archive to github.com/ctalledo/go-archive v0.2.1-0.20260717121834-2bc39549db4e. The adjacent FIXME(thaJeztah) comment explicitly marks this as a temporary testing state while waiting for moby/go-archive#45 to land.
Impact if merged: Every build of docker/cli at this commit uses code from the unreviewed personal fork (ctalledo/go-archive) rather than the canonical moby/go-archive — a supply-chain risk that defeats the stated goal of the PR. The require line correctly points to the official pre-release (v0.2.1-0.20260716170833-91b7e546daf2), but the replace directive overrides it at build time.
Resolution: Remove the replace directive (and the accompanying FIXME comment) before merging, once PR #45 in moby/go-archive is merged and the canonical module is published.
| Confidence | Score |
|---|---|
| 🟢 strong | 100/100 |
There was a problem hiding this comment.
This is temporary; upstream changes have not been merged yet
There was a problem hiding this comment.
Understood — I agree this should remain until moby/go-archive#45 is merged upstream. My finding was to flag this as blocking merge, not to suggest removing it from the branch during testing.
Once the upstream PR lands and the canonical module is published, this replace directive should be removed before merging this PR into the main branch. Until then, the temporary fork is the correct approach for validation.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
This PR vendors github.com/moby/go-archive at a dev pre-release (v0.3.0-dev / main) and applies a security-hardening layer from ctalledo/go-archive. The changes are well-structured and represent a clear improvement over the previous extraction logic.
Hardening changes reviewed:
Unpack/UnpackLayernow useos.Root(openat(2)-based) for all extraction operations, preventing symlink-escape attacks at the OS level.- Path validation replaces the old
filepath.Clean+filepath.Relbreakout check withfilepath.IsLocal+path.Clean, which is more robust. chmodNoSymlinkusesfchmodat(2)withAT_SYMLINK_NOFOLLOW(with documented fallback for older kernels that matches or improves on previousos.Chmodbehavior).lchtimesis now fd-based (unix.UtimesNanoAtvia parent directory FD), correctly scoped within the extraction root.fsRootPath/walkLinksprovide correct symlink bounding for operationsos.Rootcannot handle (mknod, xattrs) — absolute symlink targets are correctly re-bounded to root on the subsequent iteration.- The
unpackedPathskey change (absolute → POSIX-relative) is consistent:path.Cleanremoves trailing slashes before storage, and the opaque-whiteout lookup uses matching normalization. - Error-handling cleanups (
_ = writer.Close(), combined defers) are all correct.
No bugs were found in the introduced + lines.
vendor: github.com/moby/go-archive main / v0.3.0-dev
full diff: moby/go-archive@v0.2.0...91b7e54
vendor: github.com/moby/go-archive with hardening
full diff: ctalledo/go-archive@91b7e54...df2ce62
- Human readable description for the release notes
- A picture of a cute animal (not mandatory but encouraged)